-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix #132: does not sleep when it should, approach 2 #134
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I prefer if we continue with the approach shown in this PR. I am not approving yet only because there is still an issue with un-necessary sleep at max retries and a missing test for it.
I am just puzzled why the test with max retries is missing, when I am 100% sure I had it at the time.
#132: retry GitHub HTTP request when rate limited: does not sleep when it should We use ghStatus.SetSleepFn(sleepSpy) to spy on the requests to sleep for the success case. This causes, on purpose, all the test cases of TestGitHubStatusSuccessMockAPI to fail, in the assert assert.DeepEqual(t, haveSleeps, tc.wantSleeps)
#132: retry GitHub HTTP request when rate limited: does not sleep when it should We use ghStatus.SetSleepFn(sleepSpy) to spy on the requests to sleep for the failure case. his causes, on purpose, all the test cases of TestGitHubStatusFailureMockAPI to fail, in the assert assert.DeepEqual(t, haveSleeps, tc.wantSleeps)
Now that we have virtual sleep, we can use in the tests, as much as possible, the same values of prod. This reduces the cognitive load to compare tests and prod and convince oneself that it is the correct behavior.
This is a good example of how they are more explicit and leave no doubt
Parameters rateLimitRemaining and rateLimitReset are not taken into consideration when the HTTP status code is a transient error. Since settings these values in the test cases gives the impression that they influence the test, we remove them for clarity.
Test case "Any other error" covers the same logic of test case "non transient error, stop at first attempt", so we remove it.
3b99c86
to
0b2438e
Compare
@Pix4D/integration: ready for re-review. Thanks @aliculPix4D for pointing out yet another corner case. It has been longer than expected, but I think now that we can trust the tests. I also think that the retry loop is now self-evident. Using a virtual sleep reduced the test running time from 4.5 sec to circa 1.1 sec, although we went from 215 to 224 test cases :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-approving. I would just handle the case where jitter>0
also.
As mentioned, this is approach 2 at fixing #132. See also approach 1 at #133.
Compared with #133, it keeps the tests running as fast as possible and does not introduce test flakiness since it does not measure time.
To do this, I added an injectable sleep function, which I then substituted with a spy in the tests.
Best reviewed commit-per-commit, since commit 1 adds the repro and commit 2 adds the fix.
Fixes #132.